Skip to content

fix(idempotency): pass by value on idem key to guard inadvertent mutations #1090

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 2 commits into from
Apr 7, 2022
Merged

fix(idempotency): pass by value on idem key to guard inadvertent mutations #1090

merged 2 commits into from
Apr 7, 2022

Conversation

ojongerius
Copy link
Contributor

@ojongerius ojongerius commented Apr 5, 2022 •
edited
Loading

This protects against functions that inadvertently change the incoming event data

Description of changes:

Switch to using a deep copy of data. Without this, we've seen two documents written for one function invocation that (inadvertently) mutated the event data:

  • An initial document with a hashed idempotency key based on the original payload that will be stuck in INPROGRESS and has no return data
  • A second document with a hashed idempotency key based on the mutated payload with status COMPLETED and the correct return value

Happy to add a test if there is an appetite for this change.

Checklist

@pull-request-size pull-request-size bot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Apr 5, 2022
@boring-cyborg
Copy link

boring-cyborg bot commented Apr 5, 2022

Thanks a lot for your first contribution! Please check out our contributing guidelines and don't hesitate to ask whatever you need.

@ojongerius
Copy link
Contributor Author

FWIW some code to reproduce, the assertion that only one document is written fails without my patch.

Results:

'Items': [{'id': 'test-func.handler#8339d7030a6b244b42e6bb381d2c947c',
                'expiration': Decimal('1649130578'), 'status': 'INPROGRESS'},
                {'id': 'test-func.handler#5cd779b2a9dc323203f55b7ef9a6053b',
                 'data': 'null',
                 'expiration': Decimal('1649130578'),
                 'status': 'COMPLETED'}]

Test code:

def test_event_change_changes_key(dynamodb, aws_credentials):
    table = dynamodb.create_table(
        TableName='idempotency',
        KeySchema=[{"AttributeName": "id", "KeyType": "HASH"}],
        AttributeDefinitions=[{"AttributeName": "id", "AttributeType": "S"}],
        BillingMode="PAY_PER_REQUEST",
    )
    table.wait_until_exists()
    assert table.scan()['Count'] == 0

    event = {"detail": {"items": ["item_one", "item_two"]}}

    @idempotent(persistence_store=DynamoDBPersistenceLayer(table_name="idempotency"))
    def handler(event, context):
        items = event.get("detail").get("items")
        items.pop() # 👈 💣

    handler(event, {})

    scan_results = table.scan()
    print(f"ddb items:{scan_results}")
    assert scan_results['Count'] == 1

@heitorlessa
Copy link
Contributor

Hi @ojongerius! Could you please create a bug report and add a test for this?

I can see customers inadvertently missing the copy by reference nature leading to this issue - deep copy is the right call.

Once we have a bug report and a quick test, we should be able to merge it quickly.

Thank you 🙏

@heitorlessa heitorlessa changed the title fix: make hashed idempotency key stable across status transitions fix(idempotency): g Apr 5, 2022
@github-actions github-actions bot added the bug Something isn't working label Apr 5, 2022
@heitorlessa heitorlessa changed the title fix(idempotency): g fix(idempotency): pass by value copy over pointer on Apr 5, 2022
@heitorlessa heitorlessa changed the title fix(idempotency): pass by value copy over pointer on fix(idempotency): pass by value on key to guard inadvert mutations Apr 5, 2022
@heitorlessa heitorlessa changed the title fix(idempotency): pass by value on key to guard inadvert mutations fix(idempotency): pass by value on idem key to guard inadvert mutations Apr 5, 2022
@codecov-commenter
Copy link

codecov-commenter commented Apr 5, 2022 •
edited
Loading

Codecov Report

Merging #1090 (2e9797a) into develop (f50ec86) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff            @@
##           develop    #1090   +/-   ##
========================================
  Coverage    99.96%   99.96%           
========================================
  Files          119      119           
  Lines         5377     5378    +1     
  Branches       613      613           
========================================
+ Hits          5375     5376    +1     
  Partials         2        2
Impacted Files Coverage Δ
...ws_lambda_powertools/utilities/idempotency/base.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f50ec86...2e9797a. Read the comment docs.

@ojongerius ojongerius force-pushed the address_event_mutation branch from 6b21349 to 9ca6ed5 Compare April 5, 2022 23:33
@pull-request-size pull-request-size bot removed the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Apr 5, 2022
@boring-cyborg boring-cyborg bot added the tests label Apr 5, 2022
@pull-request-size pull-request-size bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Apr 5, 2022
@ojongerius
fix(idempotency): pass by value on idem key to guard against inadvert…
2e9797a 
… mutations

This protects against functions that inadvertently change the incoming event data
@ojongerius ojongerius force-pushed the address_event_mutation branch from 9ca6ed5 to 2e9797a Compare April 5, 2022 23:34
@ojongerius
Copy link
Contributor Author

Cheers @heitorlessa! I've added a test and created #1093

@heitorlessa heitorlessa mentioned this pull request Apr 6, 2022
Closed
6 tasks
heitorlessa
heitorlessa reviewed Apr 6, 2022
View reviewed changes
Copy link
Contributor

@heitorlessa heitorlessa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there's an issue with the test setup as hashed_idempotency_key is the same in both instances; meaning the test passes with or without the fix.

I'll push a fix today. We'll need to improve the build out of stub expected params (eventually moving to DynamoDB Local to reduce boilerplate for functional tests)

@heitorlessa
Copy link
Contributor

@ojongerius tests fixed - could you please have one last look before we merge?

Thanks

@ojongerius
Copy link
Contributor Author

Thanks for that 🙏

Had one last look and verified this in my own environment 👍

@heitorlessa heitorlessa merged commit 006caf6 into aws-powertools:develop Apr 7, 2022
@boring-cyborg
Copy link

boring-cyborg bot commented Apr 7, 2022

Awesome work, congrats on your first merged pull request and thank you for helping improve everyone's experience!

@ojongerius ojongerius deleted the address_event_mutation branch April 7, 2022 21:57
@heitorlessa heitorlessa changed the title fix(idempotency): pass by value on idem key to guard inadvert mutations fix(idempotency): pass by value on idem key to guard inadvertent mutations Apr 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@heitorlessa heitorlessa heitorlessa approved these changes

Assignees
No one assigned
Labels
bug Something isn't working size/M Denotes a PR that changes 30-99 lines, ignoring generated files. tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ojongerius @heitorlessa @codecov-commenter